feat - improve terminal command execution across windows and linux#247
feat - improve terminal command execution across windows and linux#247xinyi-gong wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the terminal tool implementation used by GitHub Copilot for Eclipse by making command execution more shell-aware (PowerShell/Bash), improving completion detection via OSC markers, and adding safety/quality features like cancellation and output truncation.
Changes:
- Switch Windows to
powershell.exeand Linux to/bin/bash, adding corresponding shell-integration scripts that emit Copilot OSC prompt/command-finish markers. - Move completion detection to a marker-based approach when integration is available, add Ctrl+C interruption when starting a new foreground command, and cancel running commands when the chat request is cancelled.
- Normalize/format command input (including bracketed paste for multiline) and truncate/clean terminal output before returning it to the model.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/tools/RunInTerminalToolAdapter.java | Updates tool descriptions for PowerShell/Bash, passes an initial working directory to terminal execution, and truncates/cleans output returned to the model. |
| com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java | Cancels the active terminal command when the user cancels an in-flight chat request. |
| com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/tools/RunInTerminalToolAdapterTest.java | Tightens an assertion and minor formatting cleanup. |
| com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/terminal/api/TerminalCommandProcessorTest.java | Adds unit tests for command formatting, marker-based completion, prompt-based completion, and output truncation/cleanup. |
| com.microsoft.copilot.eclipse.ui.terminal/src/com/microsoft/copilot/eclipse/ui/terminal/RunInTerminalTool.java | Implements working-directory support, marker-based completion, Ctrl+C interruption, and UTF-8 decoding for the modern terminal implementation. |
| com.microsoft.copilot.eclipse.ui.terminal.tm/src/com/microsoft/copilot/eclipse/ui/terminal/tm/RunInTerminalTool.java | Mirrors the modern terminal behavior updates for the legacy TM terminal implementation. |
| com.microsoft.copilot.eclipse.terminal.api/src/com/microsoft/copilot/eclipse/terminal/api/TerminalCommandProcessor.java | Introduces shared logic for formatting commands, detecting completion, stripping markers/control sequences, and truncating output. |
| com.microsoft.copilot.eclipse.terminal.api/src/com/microsoft/copilot/eclipse/terminal/api/ShellIntegrationScripts.java | Replaces the old sh marker constants with structured OSC marker constants + script path helpers for Bash and PowerShell. |
| com.microsoft.copilot.eclipse.terminal.api/src/com/microsoft/copilot/eclipse/terminal/api/IRunInTerminalTool.java | Extends the terminal SPI to accept an initial working directory and support cancellation of the current foreground command. |
| com.microsoft.copilot.eclipse.terminal.api/scripts/copilot-sh-integration.sh | Removes POSIX sh integration script. |
| com.microsoft.copilot.eclipse.terminal.api/scripts/copilot-powershell-integration.ps1 | Adds PowerShell integration script emitting Copilot OSC markers. |
| com.microsoft.copilot.eclipse.terminal.api/scripts/copilot-bash-integration.sh | Adds Bash integration script emitting Copilot OSC markers. |
Comments suppressed due to low confidence (2)
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/tools/RunInTerminalToolAdapter.java:84
- These text-block description lines exceed the 120-character Checkstyle LineLength limit. Please wrap/split the long sentences in this Linux tool description section to keep each physical line <= 120 chars.
Command Execution:
- Use && to chain commands on one line
- Never create a sub-shell (e.g., bash -c "command") unless explicitly asked
- Prefer pipelines | for data flow
- Multi-line commands must be complete and non-interactive. Do not run REPLs, continuation-prompt commands, \
unmatched quotes or brackets, or commands that wait for stdin. For Python/Node scripts, create a file first,
then run it
com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/tools/RunInTerminalToolAdapter.java:107
- These macOS/Unix text-block description lines appear to exceed the 120-character Checkstyle LineLength limit. Please wrap/split the long sentences so Checkstyle passes.
Command Execution:
- Use && to chain commands on one line
- Never create a sub-shell (e.g., bash -c "command") unless explicitly asked
- Prefer pipelines | for data flow
- Multi-line commands must be complete and non-interactive. Do not run REPLs, continuation-prompt commands, \
unmatched quotes or brackets, or commands that wait for stdin. For Python/Node scripts, create a file first,
then run it
| @@ -60,8 +68,9 @@ public class RunInTerminalTool implements IRunInTerminalTool { | |||
| // Output and command state | |||
| private StringBuilder sb; | |||
| private CompletableFuture<String> resultFuture; | |||
There was a problem hiding this comment.
resultFuture is read from the terminal output monitor thread but written from executeCommand() without being volatile or consistently protected by the same lock. The other command-state fields are volatile, so the monitor can observe the new command state while still seeing an old/null/completed future and skip completion detection for the current command. At minimum this should be volatile; preferably the future, active command, marker mode, and skip state should be captured in one immutable/synchronized command state so the monitor cannot observe a mixed state from two executions. The same issue exists in the TM terminal implementation copy.
There was a problem hiding this comment.
Good point. I updated both terminal implementations to avoid observing mixed command state from the output monitor thread.
The per-command data is now captured in a single volatile ForegroundCommand snapshot. I intentionally kept skipNextCompletionAfterInterrupt outside of ForegroundCommand because it represents stale terminal-buffer cleanup after Ctrl+C rather than metadata for the newly running command. The skip behavior is handled separately in the completion processor.
| return CompletionCheckResult.incomplete(); | ||
| } | ||
|
|
||
| if (skipCompletion) { |
There was a problem hiding this comment.
This skip is keyed only by a global boolean, so it discards the next complete marker region regardless of whether that region actually belongs to the interrupted command. One failure mode: Ctrl+C is sent for command A, skipNextCompletionAfterInterrupt is set, the buffer is cleared while A's stale marker is still pending, then command B completes; B's marker becomes the first complete region seen and is deleted as "skipped", leaving B's future waiting for later/unrelated output. This needs to be tied to a command generation/session marker or otherwise reset only when the interrupted command's marker is actually consumed.
There was a problem hiding this comment.
Thanks, The completion processor now checks the completed region against the active command text before applying the skip. If the next completed region belongs to command B, it contains B's command text and will not be discarded. Only a stale completed region that does not contain the active command is skipped.
Summary
This PR updates the Copilot terminal tool to use shell-aware command execution and marker-based completion across Windows and Linux.
Behavior changes
- prompt start
- prompt end
- command completion with exit code